-
Notifications
You must be signed in to change notification settings - Fork 63
Accept filter instead of exclude mask for ignored fields #265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8f46de6
to
d45b3ed
Compare
/lgtm |
@AnishShah: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/assign @thockin |
I don't think I do. I am trying to understand it and figure out how much I want to complain about the structure of it vs. accepting it as-is :) |
I don't mean to rush you. I'm thinking purely about the logistics of how to get a PR in this repo merged when it is ready. I'll hold to merge on your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote and then deleted a dozen comments, as I understood it better on each reading. I did not review all the set logic, but looking at it for the ergonomics of defining patterns.
@thockin @AnishShah feedback applied. |
673670f
to
4fa0d92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I am not approver here. I understand the design, though.
For posterity, Joe and I discussed this in real time. He tells me it's fine and does not change SSA behavior:
Given input (indent for clarity only), which has exactly two "leaf" fields:
{
"foo" /* struct */,
"foo.bar" /* struct */,
"foo.bar.bat" /* struct */,
"foo.bar.bat.qux" /* LEAF string */,
"foo.zrb", /* LEAF string */
}
Consider a strategy that has GetResetFields() => "foo.bar.bat"
. That would produce:
{
"foo", "foo.bar", "foo.zrb"
// note: "foo.bar.bat" and its children are culled
// note: "foo.bar" remains, though it is empty
}
If we inverted it to an Include("foo.zrb")
, that would produce:
{
"foo", "foo.zrb"
// note: only path elements leading to "foo.zrb" remain
}
In particular, note that "foo.bar" is absent. We didn't prune it from the old case, even though it is "empty". It didn't match anything in the new case.
In neither case did we match the "leaf" fields under "foo.bar.bat/*" (as expected).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold in case others wanna take a look.
@@ -17,6 +17,8 @@ limitations under the License. | |||
package fieldpath | |||
|
|||
import ( | |||
"fmt" | |||
"sigs.k8s.io/structured-merge-diff/v4/value" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: import sorting
/hold cancel |
/sig api-machinery
kubernetes/kubernetes#128266 needs a way of expressing reset fields that can express "exclude all fields except for the container resource fields modifiable via /resize".
This provides a way to pass a filter instead of a field exclusion set.